-
Notifications
You must be signed in to change notification settings - Fork 437
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
only add Display Search Facets to the Homepage #2275
only add Display Search Facets to the Homepage #2275
Conversation
change comment of homepage-config.interface.ts
9bfc103
to
0551229
Compare
…hub.com/GauravD2t/Advanced-search into Search-Facets-home-community-collection
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @GauravD2t : I gave this another look/test today. It's working, but some of the prior feedback has still not been addressed and tests are failing. Overall, this is looking much better and seems nearly ready... we just need to have an option to disable it.
- In his review, @artlowel noted that we should provide an option to disable this feature as not all sites will want these facets on the homepage. I had also mentioned that in the original design discussion in this comment on the original ticket. I've copied my comment below:
- "I also want to note that there are potential performance concerns with putting too much content on the homepage. At the very least, this feature needs to be possible to disable if it exists on the homepage."
- To fix this issue, I'd recommend adding a new config similar to this:
# This section already exists in `config.example.yml` homePage: # Enable or disable the Discover filters on the homepage showDiscoverFilters: true/false
- Minor issue: I've noted the lines below where tests are failing and how to fix them.
- Minor issue: There's a misalignment of code issue that I noted previously in
search-filter.component.html
. It looks like you can simply undo all changes to that file...as the only changes are to accidentally misalign the code (making the code harder to read)
Overall, this is looking good & I thank you for your hard work here.
src/app/shared/search/search-filters/search-filter/search-filter.component.html
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@GauravD2t : Thanks for your hard work on this! I'm nearly a +1 as I can verify this is working properly. However, currently our automatic accessibility tests are failing on the homepage because the "Discover" header is an <h3>
when it expects an <h2>
(as there's no <h2>
heading above that <h3>
). I think we should try to find a way to fix this within this PR if possible. (If somehow this is too difficult, we could also consider temporarily disabling the failing accessibility test -- but I'm hopeful we won't need to do that.)
More information on the accessibility error inline below.
src/app/shared/search/search-filters/search-filters.component.html
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @GauravD2t
The new configuration option works for me, but I would still disable the feature by default. I'm not a fan of how it makes the homepage take up the full width of the window either
@GauravD2t : Could you change the configuration so that this feature is disabled by default as requested by @artlowel above? Thanks. I'll try to give this another test today to make sure my feedback has been addressed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@GauravD2t : Thanks for the updates. I've given this a test today & all my prior feedback has been addressed. I think the final step is to disable this by default (as @artlowel requested above). I've added some notes inline on where those minor changes are necessary.
After these changes are made, I'll give it a final look & then squash merge it (so that it goes in as one commit instead of 70+). Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Thanks @GauravD2t ! This all looks good to me now. Merging.
Feedback was addressed. Disabled by default.
[DSC-1893] item-alert: hide redirect to home button for administrators Approved-by: Francesco Molinaro
References
Add references/links to any related issues or PRs. These may include:
Description
This PR adds Search filters to the Homepage (only). Search filters for Community/Collection pages was split off into #2732
Enabled by default, but can be disabled via this new configuration in config.yml:
Checklist
This checklist provides a reminder of what we are going to look for when reviewing your PR. You need not complete this checklist prior to creating your PR (draft PRs are always welcome). If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!
yarn lint
yarn check-circ-deps
)package.json
), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.